Skip to content

Conversation

@JamesMcClung
Copy link
Collaborator

Adds a test to test_reflective_bcs_integration.cxx that's the same as the existing one, but along z instead of y. It fails, and the bug is tracked to the current reflection of psc_bnd_fields_impl.hxx, which had a double subtraction.

This PR also makes field reflection consistently only first-order, i.e., correct to 1 ghost cell, since it's impossible to be consistently second-order with only 2 ghost cells per boundary due to the extra cell that node-aligned quantities require.

That change, plus a removal of some debug stuff, can easily be reverted.

Copy link
Contributor

@germasch germasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, staggered grids suck...

For some historical context, PSC used to always use three ghost points on either side, which I think was meant such that it covered [-2, -1, 0, ..., m-1, m, m+1] and [-1.5, -.5, .5, .., m-.5, m+.5, m+1.5] depending on staggering (so it would actually 2 ghost on the left and 3 on the right, or 2 left, 2 right, respectively).

That doesn't mean, though, that these were actually necessary/used in practice. Clearly, first order particles don't need as many ghost points, and another aside is that the ghost points kinda have double duty, being used for interpolation / current deposition, but also do advance the E&M fields, but for the latter I think one only needs to go [0.. m], IIRC.

I don't know if you thought through in particular the case of 2nd order particles (which aren't really sure to be used again anytime soon/ever). But I think it'd be preferable to keep the #ifdef DEBUG poisoning with NaNs, because that's a straightforward way to make sure one isn't using ghostpoints that haven't been set.

@JamesMcClung JamesMcClung merged commit 19c0e54 into psc-code:main Oct 24, 2025
6 checks passed
@JamesMcClung JamesMcClung deleted the pr/fix-z-reflection branch October 24, 2025 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants